-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
PoC: Show media previews in the List View via new API #57349
Conversation
Size Change: -8 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
4b9ce9e
to
526605b
Compare
526605b
to
f35b97c
Compare
Flaky tests detected in f35b97c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7302253510
|
f35b97c
to
9376f15
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is a clever idea, thanks for picking this one up @t-hamano!
Overall, I don't mind the idea of adding state to keep track of the block images, but I was wondering what the benefits or trade-offs might be between using state versus trying to find a declarative approach in block.json
that defines where the block image is stored, or something along those lines.
What I had in mind was if it'd be possible to have something in block.json
that specifies where to look for the block image. So the Image block might reference the image's id or external url, and the Gallery block might specify "children" or "innerBlocks" so that the list view knows to iterate over the first x number of blocks within the block in order to grab the image urls. That approach might have its own drawbacks or blockers, too.
Mostly, what I'm wondering is how could we simplify the API for it so that if and when we want to allow 3rd party blocks to expose images in the list view, it'd be as simple as possible for them to do so.
I suppose from my perspective, one of the drawbacks of using a useEffect
to call setBlockImage
is that it means the block itself has to know about its own images and what to do with them, so things like the background image block support won't be exposed automatically. But one of the benefits is that blocks like the image block can grab the right URL from an ID, etc, and in that case the block itself could be the right place to do it 🤔
Could there even be an in-between approach where we have a default behaviour that happens within useBlockProps
(inferred via block.json
, perhaps), but folks can pass a blockImage
prop to it to override when needed?
It's an interesting problem! What are your thoughts about the approach and how it might work for other blocks / factoring in things like the background image support? I might just ping @youknowriad for visibility, too, given recent discussions about the editor APIs.
Thank you for your feedback, @andrewserong!
I was also concerned about this. While this PR approach gives us the flexibility to expose list view image sources, it can be a bit unintuitive, especially for third-party block developers😅
Ideally, this would certainly be easier to understand and use. For example, if we want to expose the {
"name": "core/image",
"supports": {
"listViewImage": {
"attribute": "url"
}
}
} Or if we want to convert to image URL based on media Id: {
"name": "core/image",
"supports": {
"listViewImage": {
"attribute": "id",
"type": "mediaId"
}
}
} Or if we want to expose list view images based on child block attributes in the Group block: {
"name": "core/group",
"supports": {
"listViewImage": {
"type": "children",
"blocks": [ "core/image" ],
"attribute": "url"
}
}
} I'd like to hear everyone's opinions on what approach is best. |
Nice idea, and I agree with @andrewserong's comments here, I think if we are to add an API here, it should be something declarative in the register block function rather than a state. In a sense, the "image" is just another representation of a block
I'm sure I'm forgetting more, but it seems we may need Now, we already have I guess what I'm saying is that we should keep in mind all these precedents and think about something more generic if possible, if not, using the same approach and add a |
Thank you for your opinions, @andrewserong and @youknowriad. So, I'd like to close this PR and explore an approach to opt-in declaratively via |
Note: This PR is incomplete, so do not merge it.
Related to #53381
Hopefully #53684 will be resolved in the future.
What?
This PR allows the media preview in the List View implemented in #53381 to be used in blocks other than the Image block and the Gallery block.
If this approach makes sense, I would like to consider the following:
getImageUrl
,getBlockMediaPreview
etc.) and specificationsPlease let me know if this approach makes sense.
Why?
Media preview in the List View is currently limited to the Image block and the Gallery block only. To apply this feature to other blocks such as Cover, Media & Text, including third-party blocks, we cannot rely on block names or specific attributes.
How?
To make this functionality universally available, I added new APIs:
getBlockImage
/setBlockImage
. If we want to add this media preview to other blocks in the future, we could use the following implementation.Testing Instructions
The Gallery and Image blocks that support media previews have been updated to use this new API. The behavior in the List View should remain as before.